Add SphericalHarmonicsAudio classes #166

tluebeck · 2025-07-06T13:35:55Z

Which issue(s) are closed by this pull request?

Closes #165

Changes proposed in this pull request:

introduce SphericalHarmonicsAudio base class which holds all SH related properties
introduce SphericalHarmonicsTimeData and FrequencyData

f-brinkmann

Thanks for the effort. It generally looks good to me! I suggested one structural change and adding the docs but otherwise its fine to me. I think the testing should be fine. complete functionality is tested for SH signal, initialization tested for each class separately.

f-brinkmann · 2025-07-10T11:26:01Z

spharpy/classes/audio.py

+        # check dimensions
+        if len(data.shape) < 3:
+            raise ValueError("Invalid number of dimensions. Data should have "
+                             "at least 3 dimensions.")
+
+        # set n_max
+        n_max = np.sqrt(data.shape[-2])-1
+        if n_max - int(n_max) != 0:
+            raise ValueError("Invalid number of SH channels: "
+                             f"{data.shape[-2]}. It must match (n_max + 1)^2.")
+        self._n_max = int(n_max)


It seems a little redundant to me to have this in all three classes. Would the following work:

First call the init of the Audio class (FrequencyData in this case)

Second call the init of the SHAudio class and move this code there

Good point, yes that works.

f-brinkmann · 2025-07-10T11:26:25Z

spharpy/classes/audio.py

+
+
+class SphericalHarmonicTimeData(SphericalHarmonicsAudio, TimeData):
+    """_summary_


Missing docs - probably intended to get a review before the effort.

f-brinkmann · 2025-07-10T11:26:34Z

spharpy/classes/audio.py

+
+
+class SphericalHarmonicFrequencyData(SphericalHarmonicsAudio, FrequencyData):
+    """_summary_


Missing docs - probably intended to get a review before the effort.

add docstrings move n_max and dimensions check to base class

f-brinkmann

Looks good to me. I would suggest to wait with the merge until we finalized discussions on the SH conventions that we want to support. This might cause changes here as well.

spharpy/classes/audio.py

ahms5

Thank you for taking care!
just minor comments

ahms5 · 2025-07-31T22:38:28Z

spharpy/classes/audio.py

-class SphericalHarmonicSignal(Signal):
-    """Create audio object with spherical harmonics coefficients in time or
-    frequency domain.
+class SphericalHarmonicAudio(_Audio):


Suggested change

class SphericalHarmonicAudio(_Audio):

class _SphericalHarmonicAudio(_Audio):

shouldnt this be private, too?

ahms5 · 2025-07-31T22:42:44Z

spharpy/classes/audio.py

+                        comment=comment, is_complex=is_complex)
+        SphericalHarmonicAudio.__init__(
+            self, basis_type, normalization, channel_convention,
+            condon_shortley, domain="time", comment=comment)


Suggested change

condon_shortley, domain="time", comment=comment)

condon_shortley, domain=domain, comment=comment)

domain="time"would overwrite the given domain. But i think we can remove it here anyway

ahms5 · 2025-07-31T22:48:51Z

spharpy/classes/audio.py

+    def __init__(self, basis_type, normalization, channel_convention,
+                 condon_shortley, domain, comment=""):
+        _Audio.__init__(self, domain=domain, comment=comment)


Suggested change

def __init__(self, basis_type, normalization, channel_convention,

condon_shortley, domain, comment=""):

_Audio.__init__(self, domain=domain, comment=comment)

def __init__(self, basis_type, normalization, channel_convention,

condon_shortley):

i wounder if we really need it here, because it would be set twice, by the audio class and then overwritten by this class. I think the _Audio.init is not setting anything else then this to entries, so i guess we can remove it.

ahms5 · 2025-07-31T22:49:24Z

spharpy/classes/audio.py

+        SphericalHarmonicAudio.__init__(
+            self, basis_type, normalization, channel_convention,
+            condon_shortley, domain="time", comment=comment)


Suggested change

SphericalHarmonicAudio.__init__(

self, basis_type, normalization, channel_convention,

condon_shortley, domain="time", comment=comment)

SphericalHarmonicAudio.__init__(

self, basis_type, normalization, channel_convention,

condon_shortley)

sikersten · 2025-08-21T12:32:03Z

Thanks for implementing! I think I am missing something fundamental here. Why is it not possible that _SphericalHarmonicAudio inherits from the SphericalHarmonics class?

mberz · 2025-08-22T08:48:15Z

Thanks for implementing! I think I am missing something fundamental here. Why is it not possible that _SphericalHarmonicAudio inherits from the SphericalHarmonics class?

Because the SphericalHarmonics class contains a number of properties which are not meaningful in the SphericalHarmonicSignal class. They however share a set of properties/methods which would make a shared base class usefull, see #173

mberz · 2025-10-23T18:52:50Z

Sorry, forgot one thing. I would suggest to rename audio.py to sh_audio.py.

Please rename files in this pr. this will cause conflicts with other branches.

I'm very sorry, I meant to write don't rename in this branch, since the file is already part of develop.
Could you please revert dfd4d54 ?

mberz

I'm just commenting on this now, as I feel we need to make some adjustments to the SphericalHarmonicDefinition class before continuing with this PR.

I think it makes sense to move the functionality of saving the old properties to the parent class, as it could come in handy in other classes.
I now remember why I first had the SphericalHarmonicDefintion in mind without the n_max property defined. I'll add an abstract base class for that which could serve as parent for the SphericalHarmonicAudio classes.

Sorry to postpone this PR once again.

Always store fully normalized data in ACN order and renormalize accordinglz in setter/getter

f-brinkmann

Following up on the question on slack, I think it would be cleaner to keep data out of _SHAudio. Could this work:

move checks for n_max from _SHAudio.__init__ to a private method, e.g., _SHAudio._check_data_shape. This avoids the need to pass data to init.
Call this private method in each setter, e.g., SHTimeData.time
In each init, e.g., SHTimeData first initialize _SHAudio, than call the private method for checking the data shape, and finally call TimeData.__init__

Other points:

Instead of inheriting from _SphericalHarmonicBase and re-implementing the n_max getter, I think it would be cleaner to inherit from SphericalHarmonicDefinition and

overload the n_max setter to raise a ValueError, e.g., 'n_max can not be set and is defined by the shape of the data contained in the spherical harmonic Audio object'
overload the basis_type setter to raise a ValueError, e.g., 'Setting the basis_type is not yet supported'. This could be implemented at a later point, if we want. Since we decided to internally store data in standard conventions, this would be fairly straigt forward in this case.

mberz

I've added a couple of questions which may help to avoid the missing self._data problem.

mberz · 2025-11-27T08:17:09Z

spharpy/classes/audio.py

+                 condon_shortley):
+
+        # check dimensions
+        if len(self._data.shape) < 3:


Suggested change

if len(self._data.shape) < 3:

if len(self.cshape) < 2:

mberz · 2025-11-27T08:17:24Z

spharpy/classes/audio.py

+                             "at least 3 dimensions.")
+
+        # calculate n_max
+        n_max = np.sqrt(self._data.shape[-2])-1


Suggested change

n_max = np.sqrt(self._data.shape[-2])-1

n_max = np.sqrt(self._data.cshape[-1])-1

mberz · 2025-11-27T08:17:37Z

spharpy/classes/audio.py

+            raise ValueError("Invalid number of SH channels: "
+                             f"{self._data.shape[-2]}. It must match "
+                             "(n_max + 1)^2.")
+        self._n_max = int(n_max)


Suggested change

self._n_max = int(n_max)

mberz · 2025-11-27T08:18:07Z

spharpy/classes/audio.py

+    @property
+    def n_max(self):
+        """Get or set the spherical harmonic order."""
+        return self._n_max


Suggested change

return self._n_max

return int(np.sqrt(self._data.cshape[-1])-1)

mberz · 2025-11-27T08:20:11Z

spharpy/classes/audio.py


-class SphericalHarmonicSignal(Signal):
-    """Create audio object with spherical harmonics coefficients in time or
+class _SphericalHarmonicAudio(_Audio, _SphericalHarmonicBase):


Suggested change

class _SphericalHarmonicAudio(_Audio, _SphericalHarmonicBase):

class _SphericalHarmonicAudio(_Audio, _SphericalHarmonicBase, ABC):

sikersten · 2025-11-28T08:04:27Z

spharpy/classes/audio.py

+        FrequencyData.freq.fset(self, value)
+
+
+class SphericalHarmonicSignal(_SphericalHarmonicAudio, Signal):


Maybe I do not fully get the concept, but shouldn't this be

Suggested change

class SphericalHarmonicSignal(_SphericalHarmonicAudio, Signal):

class SphericalHarmonicSignal(SphericalHarmonicTimeData, SphericalHarmonicFrequencyData):

Yes, that's true. With the new inheritation structure we need that. I have it changed locally already :)

This is not obsolete, right?

manually integrate branch #229

allow to pass sh data with dimensions < 3

f-brinkmann

Thanks. Structure and docs are fine to me. I found a few small things that still should be changed.

f-brinkmann · 2025-12-04T19:48:28Z

spharpy/classes/audio.py

+    @property
+    def n_max(self):
+        """Get or set the spherical harmonic order."""
+        return int(np.sqrt(self.cshape[-1])-1)


I think overwriting the setter for basis_type must still be done to raise an AttributeError along the lines of 'Changing the basis_type' is not yet supported'.

I'll take a closer look at that point. I tried this real quick, but if we overwrite it, we cannot init _SphericalHarmonicBase because it calls the basis_type setter :D Let me check if there is a way without changing _SphericalHarmonicBase.

Found a solution - not super beautiful but works

f-brinkmann · 2025-12-04T19:52:05Z

spharpy/classes/audio.py

+    Adds a singleton dimensions at the front if necessary.
+    """
+
+    return data[np.newaxis, ...] if data.ndim < 3 else data


Without this change, a 2D array could be returned if passing a 1D array:

Suggested change

return data[np.newaxis, ...] if data.ndim < 3 else data

return np.atleast_2d(data)[np.newaxis, ...] if data.ndim < 3 else data

f-brinkmann · 2025-12-04T19:55:53Z

spharpy/classes/audio.py

+        FrequencyData.freq.fset(self, value)
+
+
+class SphericalHarmonicSignal(_SphericalHarmonicAudio, Signal):


This is not obsolete, right?

f-brinkmann · 2025-12-04T19:57:54Z

spharpy/classes/audio.py

-    def n_max(self):
-        """Get the maximum spherical harmonic order."""
-        return self._n_max
+    def freq(self):


The getter and setter for freq_raw must be overwritten as well, I think.

overwrite basis_type setter and freq_raw

add SHAudio SHTimeData and SHFrequencyData

6c64125

tluebeck changed the base branch from main to develop July 6, 2025 13:36

tluebeck self-assigned this Jul 6, 2025

tluebeck added the enhancement New feature or request label Jul 6, 2025

tluebeck marked this pull request as draft July 6, 2025 13:37

tluebeck added 2 commits July 6, 2025 15:54

add tests

700bcb9

add doc and clean up

67b163f

tluebeck requested review from ahms5, f-brinkmann, mberz and sikersten July 7, 2025 06:55

fix typo

d4e0556

mberz added this to Weekly Planning Jul 8, 2025

github-project-automation bot moved this to Backlog in Weekly Planning Jul 8, 2025

mberz moved this from Backlog to Drafting Phase in Weekly Planning Jul 8, 2025

f-brinkmann requested changes Jul 10, 2025

View reviewed changes

github-project-automation bot moved this from Drafting Phase to Require review in Weekly Planning Jul 10, 2025

tluebeck added 2 commits July 11, 2025 07:24

fix typo

90d05ba

update according to review

40a973b

add docstrings move n_max and dimensions check to base class

tluebeck requested a review from f-brinkmann July 18, 2025 11:14

f-brinkmann approved these changes Jul 24, 2025

View reviewed changes

spharpy/classes/audio.py Show resolved Hide resolved

github-project-automation bot moved this from Require review to Reviewer Approved in Weekly Planning Jul 24, 2025

f-brinkmann moved this from Reviewer Approved to Require review in Weekly Planning Jul 24, 2025

ahms5 added this to the v1.0.0 milestone Jul 25, 2025

ahms5 requested changes Jul 31, 2025

View reviewed changes

update according to review

de6eaaa

ahms5 modified the milestones: v1.0.0, v1.1.0 Sep 12, 2025

rename audio to sh_audio

dfd4d54

mberz reviewed Oct 23, 2025

View reviewed changes

revert renaming of sh_audio

6666f3a

mberz mentioned this pull request Oct 24, 2025

Abstract base class for SphericalHarmonicDefiniton and -Audio #218

Merged

tluebeck added 4 commits November 19, 2025 08:29

Merge branch 'develop' into dev/spherical_harmonics_audio

2fce301

Incorporate new SHBase class

e9c4fb0

override time and freq setter in SHaudio

cc245e2

Change normalization and channel_order behaiviour

001de12

Always store fully normalized data in ACN order and renormalize accordinglz in setter/getter

f-brinkmann self-requested a review November 26, 2025 17:15

f-brinkmann reviewed Nov 26, 2025

View reviewed changes

mberz reviewed Nov 27, 2025

View reviewed changes

sikersten reviewed Nov 28, 2025

View reviewed changes

tluebeck added 5 commits November 29, 2025 13:33

udpate according to review

25dcd87

add data init methods

9348b08

manually integrate branch #229

clean up tests

c07b683

update tests

020af4c

allow to pass sh data with dimensions < 3

fix ruff errors

b05cb04

tluebeck requested review from ahms5, f-brinkmann, mberz and sikersten December 4, 2025 17:30

clean up

5d1015b

f-brinkmann requested changes Dec 4, 2025

View reviewed changes

pyfar deleted a comment from mberz Dec 4, 2025

update according to review

4ae1205

pyfar deleted a comment from mberz Dec 4, 2025

tluebeck added 2 commits December 4, 2025 13:20

update according to review

8276700

overwrite basis_type setter and freq_raw

update tests

015e14d

mberz mentioned this pull request Dec 5, 2025

Update SphericalHarmonic*Data related inits and properties #229

Closed



		class SphericalHarmonicTimeData(SphericalHarmonicsAudio, TimeData):
		"""_summary_



		class SphericalHarmonicFrequencyData(SphericalHarmonicsAudio, FrequencyData):
		"""_summary_

	class SphericalHarmonicAudio(_Audio):
	class _SphericalHarmonicAudio(_Audio):

	condon_shortley, domain="time", comment=comment)
	condon_shortley, domain=domain, comment=comment)

	n_max = np.sqrt(self._data.shape[-2])-1
	n_max = np.sqrt(self._data.cshape[-1])-1

	return self._n_max
	return int(np.sqrt(self._data.cshape[-1])-1)

	class _SphericalHarmonicAudio(_Audio, _SphericalHarmonicBase):
	class _SphericalHarmonicAudio(_Audio, _SphericalHarmonicBase, ABC):

		FrequencyData.freq.fset(self, value)


		class SphericalHarmonicSignal(_SphericalHarmonicAudio, Signal):

	class SphericalHarmonicSignal(_SphericalHarmonicAudio, Signal):
	class SphericalHarmonicSignal(SphericalHarmonicTimeData, SphericalHarmonicFrequencyData):

	return data[np.newaxis, ...] if data.ndim < 3 else data
	return np.atleast_2d(data)[np.newaxis, ...] if data.ndim < 3 else data

Add SphericalHarmonicsAudio classes #166

Are you sure you want to change the base?

Add SphericalHarmonicsAudio classes #166

Uh oh!

Conversation

tluebeck commented Jul 6, 2025

Which issue(s) are closed by this pull request?

Changes proposed in this pull request:

Uh oh!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahms5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sikersten commented Aug 21, 2025

Uh oh!

mberz commented Aug 22, 2025

Uh oh!

mberz commented Oct 23, 2025

Uh oh!

mberz left a comment

Choose a reason for hiding this comment

Uh oh!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

mberz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sikersten Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sikersten Nov 28, 2025 •

edited

Loading